Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[luci] Introduce the parent class method to luci::Pass #14594

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tomdol
Copy link
Contributor

@tomdol tomdol commented Jan 28, 2025

This directive fixes a build break caused by a compiler error emitted when compiling with -Werror=overloaded-virtual flag switched on. The error is about the logo::Pass::run method being hidden by luci::Pass::run.

ONE-DCO-1.0-Signed-off-by: Tomasz Dolbniak [email protected]

Related issue NPU_Compiler/issues/19901

This directive fixes a build break caused by a compiler error emitted when
compiling with -Werror=overloaded-virtual flag switched on. The error is about
the logo::Pass::run method being hidden by luci::Pass::run.

ONE-DCO-1.0-Signed-off-by: Tomasz Dolbniak <[email protected]>
@tomdol tomdol force-pushed the overloaded_virtual branch from b3f60fe to 83e647c Compare January 28, 2025 14:59
tomdol added a commit to tomdol/ONE that referenced this pull request Jan 28, 2025
This directive fixes a build break caused by a compiler error emitted when
compiling with -Werror=overloaded-virtual flag switched on. The error is about
the logo::Pass::run method being hidden by luci::Pass::run.

This is a backport of Samsung#14594
to the release branch.

ONE-DCO-1.0-Signed-off-by: Tomasz Dolbniak <[email protected]>
@seanshpark
Copy link
Contributor

@lemmaa , what is the guide for putting internal server URLs?

@@ -28,6 +28,8 @@ namespace luci
class Pass : public logo::Pass
{
public:
// This directive prevents emitting a compiler erro triggered by -Werror=overloaded-virtual
using logo::Pass::run;
Copy link
Contributor

@seanshpark seanshpark Feb 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why add this here.
I cannot accept the resolution method. please find another way of solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in fact the solution. The reason this error occurs in the first place is that the luci::Pass::run(luci::Module *) has a different signature than the parent's loco::Pass::run(loco::Graph *) and this can cause overload resolution problems when compiling the code in certain situations. This is why newer compilers report a warning and the -Werror=overloaded-virtual makes it an error which basically blocks the build on Ubuntu 24.

The using directive adds the logo::Pass::run method to the overload set of the luci::Pass class which makes the warning(error) go away and most importantly - it doesn't break anything.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO you got it wrong. luci::Pass::run(luci::Module *) is also pure virtual so that inherit class SHOULD also override.
You can inherit from loco::Pass if do not want to use luci::Module * parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original build error (screenshot in the linked issue) comes from a file CircleNodePass.h which includes the logo/Pass.h header and contains a class which inherits from logo::Pass. This is correct and matches what you said above - CircleNodePass overrides run(loco::Graph* graph) and doesn't touch the version which operates on luci::Module.

Yet the compiler produces an error about luci::Pass::run hiding logo::Pass::run withing this compilation context.

Here's a smaller snippet that demonstrates how hiding the virtual methods interferes with name lookup and overload resolution https://ideone.com/lI3QNL

Uncomment the using A::run; line fixes the problem and this is exactly what I'm proposing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only other solution I can imagine is renaming the conflicting methods so that they don't hide each other:

logo::Pass::run(Graph*) -> logo::Pass::run_on_graph(Graph*)
luci::Pass::run(Module*) -> luci::Pass::run_on_module(Module*)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried overriding luci::Pass::run(luci::Module *) ? something like

   bool run(luci::Module *) override { throw "NYI"; }

@seanshpark
Copy link
Contributor

I assume this issue is to fix for U24.04 cause of your situation.
Current ONE/compiler does not have CI for U24.04.
I cannot confirm this check is appropriate for the issue in internal compiler.
Personally I do not want this patch to land.

What I can suggest is written in #14589 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants